Add retry to StandardValueSet query to fix network errors #652
Conversation
…forcedotcom/source-deploy-retrieve into ew/standard-value-set-connection-issues
| command: | | ||
| npm install shx -g | ||
| shx rm -rf node_modules/@salesforce/source-deploy-retrieve | ||
| working_directory: node_modules/@salesforce/source-tracking |
There was a problem hiding this comment.
Is this a common thing to do for circular deps?
Does this not mean that for the tests source-tracking would be using the current version of SDR, but after deploy it'd be using whatever version it specifies in it's package.json?
There was a problem hiding this comment.
they're not circular, just transitive. plugin-source depends on SDR and STL and STL depends on SDR.
When we build the CLI, we bump both SDR and STL via yarn resolutions to their latest version.
This tests the changes on this branch everywhere that SDR is used. Imagine a bug in SDR that causes problems for STL, but you don't catch it because the nuts are using an STL with a different (bug-free) SDR. Wouldn't want to miss that!
|
|
||
| if (error.lastError && error.lastError.message) { | ||
| this.logger.debug(error.lastError.message); | ||
| } |
There was a problem hiding this comment.
Should we add a few unit tests to cover the conditions added here?
- retries and succeeds
- Hits the retry limit (seemed a bit like exercising the retry lib, but would be good to have a test to verify what happens when it hits the limit). Could just stub the failure condition early verse having to actually call until the lib fails.
- Not Retryable error thrown
It would make the unit testing a bit cleaner if you refactored the standardValueSetRecord specific handling out into a method
There was a problem hiding this comment.
Added some tests @gbockus-sf 👍
| shiki "^0.10.1" | ||
|
|
||
| typescript@^4.1.3, typescript@^4.4.3: | ||
| typescript@^4.1.3, typescript@^4.4.3, typescript@^4.7.4: |
There was a problem hiding this comment.
Seems like several of the deps are now out of sync with what's in @salesforce/dev-scripts. Just verifying that's the way to go.
|
@iowillhoit I know these problems from the past. However, I have not seen this issue since api version 50. What error messages appear in the debug log? |
src/resolve/connectionResolver.ts
Outdated
| throw err; | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
if you return the result from the try {} block, that would avoid the let , the assignment, and possibly the await
@jayree Here are the two types of errors I am seeing: Connection Reset: |
QA NOTES:✅ : following instructions in post, generated manifest 5 times, no differences |
What does this PR do?
When running
force:source:manifest:create --fromorg, thesingleRecordQuerymethod to retrieveStandardValueSetwas having connection errors, added retry logic.What issues does this PR fix or reference?
@W-11303606@
Functionality Before
Connection errors would cause some queries to fail. The result was different package.xml file almos every time you ran
force:source:manifest:create --fromorg YOUR_ORGpackage-xml-diffs-720.mov
Functionality After
Failed queries retry, the package.xml is the same every time
Testing
source-deploy-retreivebranchyarn buildyarn link(if needed)plugin-sourceyarn link @salesforce/source-deploy-retrievesfdx plugins:link .git clone git@github.com:trailheadapps/dreamhouse-lwc.git dreamhouse-testing-standard-value-setcd dreamhouse-testing-standard-value-setsfdx force:org:create -s -d 1 -f config/project-scratch-def.jsonsfdx force:source:manifest:create --fromorg THE_USERNAMEpackage.xmlgit diffto confirm not changes occur over multiple runsNow unlink everything! 🔥🔗